feat: return supported protocols in id output#7409
Conversation
|
I was trying to do this the other day, this would be great to have support for. |
Otherwise, the ID is going to be incorrect. Note: technically, the previous logic didn't need to connect to the target peer to complete. However, I'm fine dropping that capability in favor of more up-to-date information.
aschmahmann
force-pushed
the
feat/id-list-protocols
branch
from
9f678a1 to
34c0e5c
Compare
aschmahmann
left a comment
aschmahmann
left a comment
There was a problem hiding this comment.
@Stebalien made a few changes and left some comments.
@Stebalien @achingbrain are we ok merging this change without the corresponding js-ipfs one, or do we need to block on that?
| // We need to actually connect to run identify. | ||
| err = n.PeerHost.Connect(req.Context, peer.AddrInfo{ID: id}) |
There was a problem hiding this comment.
This is a fine conceptual change. Just a note that this doesn't actually do anything as the DHT (our only PeerRouting system) will connect to the peer, and will not return any addresses if it fails. https://github.com/libp2p/go-libp2p-kad-dht/blob/fc3558cc35274f5d1727dad8201f821df1ee6317/routing.go#L675-L682
There was a problem hiding this comment.
Hm. I thought I had some trouble where we weren't actually waiting for the connect event to finish.
But honestly, we should do this anyways. We shouldn't assume that the router will always end up connecting to the target peer.
| for _, a := range addrs { | ||
| info.Addresses = append(info.Addresses, a.String()) | ||
| } | ||
| sort.Strings(info.Addresses) |
There was a problem hiding this comment.
I added address sorting to this function, any objections?
| for _, a := range addrs { | ||
| info.Addresses = append(info.Addresses, a.String()) | ||
| } | ||
| sort.Strings(info.Addresses) |
There was a problem hiding this comment.
I added address sorting to this function, any objections?
No need to block on JS, I created ipfs/js-ipfs#3219 to track adding support there. |
|
This is being added to js in ipfs/js-ipfs#3250 |
4 participants
This is really useful for debugging.
TODO: fix tests if @aschmahmann and @achingbrain OK this change.